Codecov Report

Attention: Patch coverage is 90.57592% with 54 lines in your changes are missing coverage. Please review.

Project coverage is 70.1%. Comparing base (f203b03) to head (7de7f09).
Report is 21 commits behind head on main.

❗ Current head 7de7f09 differs from pull request most recent head 7cfca6d. Consider uploading reports for the commit 7cfca6d to get more accurate results

@@           Coverage Diff           @@
##            main   #1628     +/-   ##
=======================================
+ Coverage   69.3%   70.1%   +0.7%     
=======================================
  Files        136     136             
  Lines      19637   20024    +387     
=======================================
+ Hits       13610   14037    +427     
+ Misses      6027    5987     -40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

One open question I've got is whether I need to deal with deduplication when constructing the attributes list, or whether that's dealt with elsewhere.

As of today, its not done anywhere in the API, SDK, Exporter. There is a good chance of a feature flag/option to make it happen. (unsure where,, could be just in otlp exporter only)

I've opted to use serde here to convert log's structured values into AnyValues.

mm.. This may require some discussion.. The log-bridge only depends on opentelemetry (which is otel's API crate) and it does not (and should not) deal with concerns such as serialization etc. That is left for the exporters (which are based off of the opentelemetry-sdk crate).

The serialization via serde is really just an internal conversion from log's value type into AnyValues. log doesn't define its own complete data-model, instead deferring to a serialization framework like serde to provide one for it. From there, an exporter will receive AnyValues which it can choose to serialize however it wants.

If we didn't want to force a dependency on serde here (which is perfectly reasonable) we can still convert primitive types coming from log into AnyValues via its own simplified visitor API, but won't be able to convert complex types like arrays or objects without needing to format them as strings. I could make the serde dependency optional for anyone that wants to opt-in to complete support.

If we didn't want to force a dependency on serde here (which is perfectly reasonable) we can still convert primitive types coming from log into AnyValues via its own simplified visitor API, but won't be able to convert complex types like arrays or objects without needing to format them as strings. I could make the serde dependency optional for anyone that wants to opt-in to complete support.

+1 on that. As of now we only support primitive types with tokio-tracing appender, so should be good to have similar support with log-appender, and then using serde optionally for complete support.

Ok, I can make that change 👍 Any preference on what that feature should be called? log_attributes_full? The tracing appender may end up with an equivalent feature once they finish integrating valuable, unless they make that dependency required in tracing.

In the other create(opentelemetry-proto) we have the feature with_serde to annotate proto generated files with serde attributes. Maybe we can borrow it here

Ok, I've made the serde dependency optional, gated by a with_serde feature. This test case gives you a reasonable idea of how values are converted with and without the feature enabled.

I can improve the test coverage here once we’re happy with the overall shape of things. Is there anywhere I’d need to plug this new feature into CI?

Some relevant comments here open-telemetry/opentelemetry-specification#3938

Thanks for the review @lalitb. I’ll make those changes, fill out the test coverage, and leave some docs on how to enable key-value support, and how various kinds of values are represented.

This should be ready for another review now. I've added some crate-level docs to opentelemetry-appender-log. I tried to follow any conventions I saw in other docs, but please point out anywhere I should be doing things differently.

The build failure here looks unrelated; is kicking off a fresh build likely to resolve it?